-
Notifications
You must be signed in to change notification settings - Fork 14
IPA-112: Field names must use camelCase #589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| name: 'invalid schema', | ||
| document: { | ||
| components: { | ||
| schemas: { | ||
| SchemaName: { | ||
| properties: { | ||
| UserId: { type: 'string' }, | ||
| user_id: { type: 'string' }, | ||
| 'user-id': { type: 'string' }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Does it also catch nested schemas, and arrays etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It should catch nested schemas. I can add a unit test to be sure.
- What do you mean by catching arrays in this context? Can you give an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
For arrays:
ArraySchema: {
type: 'array',
items: {
properties: {
INVALID_FIELD: {},
},
},
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean the properties in items. Sure, let me add unit tests covering both and see how it works
| - Reports any instances where these field names are not in camelCase format | ||
| message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-112-field-names-are-camel-case' | ||
| severity: warn | ||
| given: '$.components.schemas..properties[*]~' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: WDYT about also targeting inline schemas? Or should we only check component.schemas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question! We should clarify what we mean by "field." To me, a field is a property of a resource schema, and resource schemas are typically defined under components.schemas. However, when I think about what I’d consider a resource schema, I’d primarily refer to response or requestBody content schemas as well.
So, are we specifically talking about inline schemas within those? Or, do we also count inline schemas used for query parameters? I’ve noticed that query parameters often use inline schemas, but they fall into a gray area—they're not path parameters, (the IPA for path params does not apply to them) and I’m not sure if their properties should be considered as fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think it could be valuable to catch fields in requests and responses as well, I'm working on the description IPA rule now and included them like so:
'$.paths[*][get,put,post,delete,options,head,patch,trace]..content..properties[*]'
In terms of query parameters, it's a good point. We don't talk a lot about them in the IPAs, but I'm thinking they should also be camelCase. Perhaps we can bring a new guideline for this. I think we can leave them out of this PR at least
Proposed changes
Jira ticket: CLOUDP-271999
Validates schema field names using the Spectral
casingfunctionDrive-by fix:
IPA-112: Avoid project field nameswill also check requestBody and response schemas similar toField names must use camelCaseChecklist
Changes to Spectral
Further comments